add rustfmt support for cfg_select
#144323
Conversation
|
Failed to set assignee to
|
|
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
rustbot
added
the
S-waiting-on-review
|
@ytmimi seems like I can't officially make you a reviewer, so we'll have to figure out how to merge this: I can |
|
Maybe you can't officially assign me, but I should definitely have the ability to |
|
I think we might want to hold off on merging this for a tad longer. The style team has been discussing possible formatting prescriptions for the macro in our weekly meetings and while we're getting closer there's still a few things we're trying to iron out - rust-lang/style-team#201 My preference would be to give that another week or two to hopefully avoid double code churn |
|
I think the proposed formatting looks reasonable, as far as the examples go. @folkertdev Could you please add some examples that include very long left-hand sides in Please include both examples where the line is overlong, and examples where the user manually wrapped the left-hand side. |
folkertdev
force-pushed
the
cfg-select-builtin-macro-fmt
branch
3 times, most recently
from
5b15896 to
89a83da
Compare
|
I added some formatting examples. They do in fact already format nicely. I did just notice that comments on the branches, like below, disappear upon formatting cfg_select! {
// This comment will disappear when formatting.
any(true) => {}
}For Does |
|
For context, @joshtriplett and I talked about this in the style team meeting, and @folkertdev, if you're interested in working with us on it, we were interested in using this PR to explore possible solutions to the questions we had raised about how to format this, such as what Josh mentioned about long left-hand sides. If we can work out some good rules in this PR, and use the test cases in it to verify that we're happy with the formatting of all the hard cases that we can think of, then we can write down the rules in the style guide around the behavior that's proposed and that we agree on here. Generally, our preference, if possible, is some wrapping rule for the LHS that keeps things below the maximum column width and that we can later apply, over a style edition, to normal |
folkertdev
force-pushed
the
cfg-select-builtin-macro-fmt
branch
from
89a83da to
08240dd
Compare
|
well, I'd like to see I added some of the long unbreakable line examples, just so we track their behavior. There we also see that comments inside of the block are dropped which is a bit strange because there we just give a tokenstream to the formatter, and I would have assumed that it handled comments. For the simple cases I think the current behavior is what we want right? though I assume the |
folkertdev
force-pushed
the
cfg-select-builtin-macro-fmt
branch
from
08240dd to
ccfa107
Compare
This comment has been minimized.
This comment has been minimized.
The trouble is that rustfmt's stability policy is that once it formats a construct, the formatting must remain byte-identical within that style edition. So whatever our behavior for the non-simple cases ends up being what we have to commit to and document in the style guide. So it's better if we can come up with reasonable rules for the non-common cases as well, so that we can commit to reasonable behavior for those cases too. We're open to and interested in seeing proposed behavior there as part of this PR. |
folkertdev
force-pushed
the
cfg-select-builtin-macro-fmt
branch
from
ccfa107 to
17d516f
Compare
src/tools/rustfmt/src/macros.rs
Outdated
| // The cfg plus ` => {` should stay within the line length. | ||
| let rule_shape = shape | ||
| .sub_width(" => {".len()) | ||
| .max_width_error(shape.width, span)?; |
There was a problem hiding this comment.
This does not seem to have the desired effect, am I using this right?
| any(feature = "acdefg", true, true, true, true, true, true, true, true) => { | ||
| compile_error!("foo") | ||
| } | ||
| any(feature = "acdefgh123", true, true, true, true, true, true, true, true) => { | ||
| compile_error!("foo") | ||
| } | ||
| any( | ||
| feature = "acdefgh1234", | ||
| true, | ||
| true, | ||
| true, | ||
| true, | ||
| true, | ||
| true, | ||
| true, | ||
| true | ||
| ) => { | ||
| compile_error!("foo") | ||
| } |
There was a problem hiding this comment.
currently this breaks when the cfg part exceeds 80 characters. I've tried to take the => { into account, but that doesn't seem to have any effect. Also when the rhs really is empty, we'd need to consider => {} for the length.
The above still has issues with comments too, which I'm not sure how to deal with.
So @ytmimi @calebcartwright a rustfmt review is probably also a good idea at this point?
There was a problem hiding this comment.
I took a very brief look at this the other day and I had some suggestions for changes. I'll do my best to wrap up my review soon.
|
#145233 will necessitate an update here. Also, there should be an update to the style guide, though that can wait until after this PR is working. |
jieyouxu
added
A-rustfmt
@ytmimi it's because rustfmt team doesn't yet have write access on r-l/r despite having r+ permissions. See rust-lang/team#1948. |
Autolabel `src/tools/{rustfmt,rust-analyzer}` changes with `T-{rustfmt,rust-analyzer}`
Make e.g. rust-lang#144323 more obvious who should be reviewing it and easier to filter.
|
We talked about this in the style team call today. Specifically, we looked at the formatting of this test case: cfg_select! {
feature = "debug-with-rustfmt-long-long-long-long-loooooooonnnnnnnnnnnnnnnggggggffffffffffffffff"
=> {
// abc
}
feature = "debug-with-rustfmt-long-long-long-long-loooooooonnnnnnnnnnnnnnnggggggffffffffffffffff"
=> {
// abc
}
_ => {
// abc
}
}What the PR currently does is this: cfg_select! {
feature = "debug-with-rustfmt-long-long-long-long-loooooooonnnnnnnnnnnnnnnggggggffffffffffffffff" => {}
feature = "debug-with-rustfmt-long-long-long-long-loooooooonnnnnnnnnnnnnnnggggggffffffffffffffff" => {}
_ => {}
}What we'd like to do instead, when the maximum line length is exceeded, is to break before cfg_select! {
feature
= "debug-with-rustfmt-long-long-long-long-loooooooonnnnnnnnnnnnnnnggggggffffffffffffffff"
=> {
println!();
}
feature
= "debug-with-rustfmt-long-long-long-long-loooooooonnnnnnnnnnnnnnnggggggffffffffffffffff"
=> {}
anything(
"some other long long long long long thing long long long long long long long long long long long",
) => {}
}One thing that motivated us here is that https://doc.rust-lang.org/nightly/reference/conditional-compilation.html#r-cfg.options.crate It's just a convention. Therefore, what's on the LHS there (even setting aside the effects of Notably, breaking before the Similarly, we don't currently line break normal attributes in this way, but we want to do for For editions, traditionally for lang work,, we've prioritized inter-edition consistency over intra-edition consistency. So, e.g., when we introduced features like RPITIT to older editions, we did so with the Rust 2024 lifetime capture rules even before we'd shipped Rust 2024 or stabilized the behavior in the nightly edition. That's similar to the model here. Since we know we want to change how we handle attributes, we'll do it first here, with @calebcartwright, I think we may have settled parts of this after you had to drop, so please let us know if anything doesn't sound right here. @folkertdev, if you could update the PR and the tests accordingly, that'd be good. We'll also need a PR to the style guide for this. Note as well that these will need to be updated to account for the change in: Thanks for working on this. |
so that we can experiment with new formatting rules for long lines
folkertdev
force-pushed
the
cfg-select-builtin-macro-fmt
branch
from
17d516f to
f3acdc9
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I added the new formatting rule for long key-value meta items. The implementation is a bit hacky though. I'd really appreciate some rustfmt feedback on the general approach here, and whether we can break anything out (maybe the refactor of the I also have some practical rustfmt implementation questions:
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| overflow::rewrite_with_parens( | ||
| context, | ||
| &path, | ||
| list.iter(), | ||
| shape | ||
| .sub_width("]".len()) | ||
| .max_width_error(shape.width, this.span)?, | ||
| this.span, | ||
| context.config.attr_fn_like_width(), | ||
| Some(separator_tactic), | ||
| ) |
There was a problem hiding this comment.
this returns an Unknown error when the list items are too long, e.g.
anything(
"some other long long long long long thing long long long long long long long long long long long",
) => {
// abc
}I'm not sure what to do with that really: is just bailing completely the right approach, or is there some sort of way to still format this in a somewhat reasonable fashion?
tracking issue: #115585
Done here because
rust-lang/rustfmthas not been synced for a while and uses a nightly version wherecfg_select!is not yet a builtin macro.r? @ytmimi